Initial Spinner Implementation by tomi-msft · Pull Request #22207 · microsoft/fluentui

tomi-msft · 2022-03-24T20:59:29Z

This PR is for the initial implementation of the v9 Spinner

Related Issue(s)

codesandbox-ci · 2022-03-24T21:04:50Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a83054e:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

fabricteam · 2022-03-24T21:05:37Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-spinner` Spinner	`6.815 kB` `2.9 kB`	`16.459 kB` `5.549 kB`	`9.644 kB` `2.649 kB`

_{🤖 This report was generated against 374dca7b9d662f71a1f81297d7e4f4ea95615645}

size-auditor · 2022-03-24T21:12:40Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 374dca7b9d662f71a1f81297d7e4f4ea95615645 (build)

…c-react into react-spinner-implementation

spmonahan · 2022-03-28T22:21:16Z

+    disabledTests: [
+      'component-has-static-classname',
+      'component-has-static-classname-exported',
+      'component-has-static-classnames-object',


I believe Spinner should be exporting the classnames object covered by this conformance test.

spmonahan · 2022-03-28T22:24:44Z

+  return (
+    <slots.root {...slotProps.root}>
+      {slots.label && (labelPosition === 'above' || labelPosition === 'before') && <slots.label {...slotProps.label} />}
+      {slots.spinner && <slots.spinner {...slotProps.spinner} />}


Seems like the spinner slot should be required as it's the core feature of this component. If so, it should be marked as NonNullable in the .types.ts file.

spmonahan · 2022-03-28T22:31:27Z

+ */
+export const useSpinner_unstable = (props: SpinnerProps, ref: React.Ref<HTMLElement>): SpinnerState => {
+  // Props
+  const { appearance = 'primary', labelPosition, size = 'medium', status = 'active' } = props;


Does the design have a default labelPosition? Seems like we should probably set one here because with it being undefined like this it will prevent labels from rendering unless a user sets labelPosition.

spmonahan · 2022-03-28T22:33:52Z

 export * from './renderSpinner';
 export * from './useSpinner';
 export * from './useSpinnerStyles';
+export * from './DefaultSvg';


What's your thinking about exporting this SVG? My initial reaction is that we should not export it and keep it an internal implementation detail. For example, we might want to move away from using an SVG for the animation.

spmonahan · 2022-03-28T22:35:14Z

+  /**
+   * The status of the Spinner. Defaults to active
+   */
+  status?: 'active' | 'inactive';


How is status different from disabled?

spmonahan · 2022-03-28T22:53:42Z

+      animationTimingFunction: 'linear',
+      backgroundColor: 'transparent',
+    },
+    [`& > svg > circle`]: {


What do you think about selecting elements based on class name rather than tag name?

spmonahan · 2022-03-28T23:00:47Z

+
+  // global class for Spinner track
+  spinnerTrack: {
+    [`& > svg > circle#track`]: {


We should not use ids as there may be multiple Spinners on a page and users may reasonably use an id like track. Using an id in a selector also makes styles harder to override because of their higher specificity.

spmonahan · 2022-03-28T23:02:22Z

+  // modifier class for Spinner track if appearance="inverted"
+  spinnerTrackInverted: {
+    [`& > svg > circle#track`]: {
+      stroke: 'rgba(255,255,255,0.2)',


Can we use a token here?

sopranopillow · 2022-03-28T23:05:25Z

+    disabledTests: [
+      'component-has-static-classname',
+      'component-has-static-classname-exported',
+      'component-has-static-classnames-object',


Since you are exporting the classnames object, this test should pass and shouldn't be disabled. Only component-has-static-classname should be disabled.

sopranopillow · 2022-03-28T23:05:47Z

+  spinner: 'fui-Spinner__spinner',
+  label: 'fui-Spinner__label',
+};
+
+const rvalues = {
+  tiny: '9',
+  extraSmall: '11',
+  small: '13',
+  medium: '14.5',
+  large: '16.5',
+  extraLarge: '18.5',
+  huge: '20',
+};
+// Sizes for the Spinner
+const spinnnerSizes = {
+  tiny: '20px',
+  extraSmall: '24px',
+  small: '28px',
+  medium: '32px',
+  large: '36px',
+  extraLarge: '40px',
+  huge: '44px',
 };
+
+// Stroke widths for the Spinner
+const spinnerStrokeWidth = {
+  sWidth: '2px',
+  mWidth: '3px',
+  lWidth: '4px',
+};
+


If these sizes are going to be added to the theme tokens, maybe add a TODO mentioning they should be replaced once the tokens are added.

sopranopillow · 2022-03-28T23:07:46Z

+      animationDuration: '1.5s',
+      animationIterationCount: 'infinite',
+      animationTimingFunction: 'cubic-bezier(0.33,0,0.67,1)',
+      //zIndex: 999,


if this isn't needed, might want to remove the comment?

spmonahan · 2022-03-30T23:47:01Z

+   * The root of the Spinner.
+   * The root slot receives the `className` and `style` specified directly on the `<Spinner>`.
+   */
  root: Slot<'div'>;


nit: Can you make this NonNullable?

spmonahan · 2022-03-30T23:53:24Z

+  const styles = useStyles();
+  return (
+    <div className={styles.container}>
+      <Spinner labelPosition="before" label={<Label>Label Position Before...</Label>} />


You should just put the label text here. No need to the <Label> since the label slot renders that for you. Doing it the way you currently have results in nested <Label>s

spmonahan · 2022-03-30T23:55:10Z

+const useStyles = makeStyles({
+  container: {
+    '> div': { ...shorthands.padding('20px') },
+    '& label': { display: 'block', marginBottom: '10px' },


I think you want to remove this style from the story as it's affecting the vertical positioning of the labels that are part of Spinner which misaligns the labels.

sopranopillow · 2022-03-31T16:21:48Z

+import * as React from 'react';
+
+export const DefaultSvg = () => (
+  <svg role="progressbar">


@tomi-msft I think you're missing this change?

sopranopillow · 2022-03-31T16:24:13Z

-  // TODO Add things shared between props and state here
+  /**
+   * The appearance of the Spinner.
+   * @defaultValue 'primary'


I think these @defaultValue should be @default based on this comment: #22096 (comment)

sopranopillow · 2022-03-31T16:25:10Z

+    <circle className="fui-spinner__Track" />
+    <circle className="fui-spinner__Tail" />


nit:

Suggested change

<circle className="fui-spinner__Track" />

<circle className="fui-spinner__Tail" />

<circle className="fui-Spinner__Track" />

<circle className="fui-Spinner__Tail" />

sopranopillow · 2022-03-31T16:34:08Z

+  // style for label
+  spinnerLabel: {
+    color: tokens.colorNeutralForeground1,
+  },
+


you can remove this since label by default uses colorNeutralForeground1

Suggested change

// style for label

spinnerLabel: {

color: tokens.colorNeutralForeground1,

},

GeoffCoxMSFT · 2022-03-31T22:43:52Z

+   * Where the label is positioned relative to the Spinner
+   * @default 'after'
+   */
+  labelPosition?: 'above' | 'below' | 'before' | 'after';


above and below feel positional - like top and bottom. Consider only before and after and then a separate property about horizontal or vertical layout.

This was discussed in the Spec PR to keep it this way to align with react-positioning: #21336 (comment)

GeoffCoxMSFT · 2022-03-31T22:45:31Z

 * Spinner Props
 */
-export type SpinnerProps = ComponentProps<SpinnerSlots> & SpinnerCommons;
+export type SpinnerProps = Omit<ComponentProps<SpinnerSlots>, 'size'> & Partial<SpinnerCommons>;


I think it is better to have commons have the required/optional based on its usage with SpinnerProps. Then have SpinnerState make required/optional changes. This way a caller reading the code doesn't have to do mental gymnastics to figure out what is required/option in props.

GeoffCoxMSFT · 2022-03-31T22:53:13Z

+
+  // modifier class for Spinner trail
+  spinnerTailInverted: {
+    [`& > svg > circle.fui-spinner__Tail`]: {


Nit: You should use regular single quote instead of string template quotes unless you are injecting a value.

GeoffCoxMSFT · 2022-03-31T22:54:00Z

+      },
+      animationDuration: '1.5s',
+      animationIterationCount: 'infinite',
+      animationTimingFunction: 'cubic-bezier(0.33,0,0.67,1)',


Consider referencing constants like you do for body1 unless the tokens are now available.

GeoffCoxMSFT · 2022-03-31T22:55:43Z

+    disabledTests: ['component-has-static-classname', 'component-has-static-classname-exported'],
  });

  // TODO add more tests here, and create visual regression tests in /apps/vr-tests


Consider adding the it('renders') test maybe with and without the label.

GeoffCoxMSFT · 2022-03-31T22:56:10Z

+ * TODO: Update with proper tokens when added
+ * Radii for the Spinner circles
+ */
+const rvalues = {


Nit: rValues

Initial Spinner Implementation

38c4714

msft-fluent-ui-bot added the Component: Spinner label Mar 24, 2022

msft-fluent-ui-bot assigned JustSlone Mar 24, 2022

tomi-msft added 7 commits March 25, 2022 02:40

Update styling and useSpinner

19e3cf0

update index files

de37957

api update

6eb6961

Make spinner and label slots optional and update api

4cdfb16

Add react-label dependency

d54a4e5

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

a873578

…c-react into react-spinner-implementation

update conformance tests and snapshots

98d93a7

tomi-msft marked this pull request as ready for review March 28, 2022 16:58

tomi-msft requested a review from a team as a code owner March 28, 2022 16:58

spmonahan reviewed Mar 28, 2022

View reviewed changes

sopranopillow reviewed Mar 28, 2022

View reviewed changes

tomi-msft added 7 commits March 29, 2022 17:16

Add stories and update styling

a604688

api update

0608aa6

update tests

1675276

Update classnames

0941f0a

update snapshots

64fa166

update tests

02fedb7

update tests

e5f8a74

spmonahan reviewed Mar 30, 2022

View reviewed changes

tomi-msft added 2 commits March 30, 2022 17:43

Add NonNullable to root and update stories

cce0d5a

Update appearance story

68f30dd

sopranopillow reviewed Mar 31, 2022

View reviewed changes

tomi-msft added 2 commits March 31, 2022 15:22

Update styles

c94bb18

Add svg className

17a6bca

GeoffCoxMSFT reviewed Mar 31, 2022

View reviewed changes

GeoffCoxMSFT approved these changes Mar 31, 2022

View reviewed changes

tomi-msft added 2 commits March 31, 2022 16:23

Update snap and styling

f58a8b4

update styling

a83054e

tomi-msft merged commit aa573af into microsoft:master Apr 1, 2022

tomi-msft mentioned this pull request Apr 5, 2022

Spinner Convergence #21399

Closed

33 tasks

		<circle className="fui-spinner__Track" />
		<circle className="fui-spinner__Tail" />

Uh oh!

Conversation

tomi-msft commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue(s)

Uh oh!

codesandbox-ci Bot commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

size-auditor Bot commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sopranopillow Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopranopillow Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffCoxMSFT Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tomi-msft commented Mar 24, 2022 •

edited

Loading

codesandbox-ci Bot commented Mar 24, 2022 •

edited

Loading

fabricteam commented Mar 24, 2022 •

edited

Loading

size-auditor Bot commented Mar 24, 2022 •

edited

Loading

sopranopillow Mar 28, 2022 •

edited

Loading

sopranopillow Mar 31, 2022 •

edited

Loading

GeoffCoxMSFT Mar 31, 2022 •

edited

Loading